-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Change YAML style of scheduler-kubeconfig from human to compact JSON #523
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: omertuc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
146c101
to
dd6cc3c
Compare
…JSON (This is currently a WIP PR suggestion) # Background The `scheduler-kubeconfig` configmap managed by this operator contains a YAML formatted `kubeconfig` string. As part of a particular novel form of OCP deployment (image based upgrade/installation), we have to programmatically modify the `server:` entry in that kubeconfig. This is easily done by parsing the YAML, editing the server entry and serializing the YAML back into the configmap. # Issue Every YAML serializer has its own opinions of what YAML should look like. When we perform the procedure described above, we get a kubeconfig that's slightly different then what this operator outputs (different indentation, different quoting decisions). As a result, it eventually causes the kube-scheduler-operator to issue a new revision of the scheduler. Rolling out the new revision takes a few minutes, which we would prefer to avoid. # Solution In order to solve this we suggest the change in this commit which would be to format this kubeconfig as unopinionated compact JSON as opposed to human readable YAML # Alternative solutions We could have a textual "search-and-replace" for the `server:` entry in our code, but we would prefer to do it right by properly parsing, editing then serializing it.
@omertuc: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Would you please elaborate more on this novel form?
Only KS-operator should be allowed to modify the kubeconfig. Otherwise, any subsequent revision roll out will undo your changes. Is there a card tracking your work? |
The tl;dr is we install SNO OCP, take a snapshot of the filesystem, and reuse that snapshot to quickly spin up new clusters, after a bit of reconfiguration For more information we're at
True, for now we're going around changing stuff we "shouldn't", in the long term we're thinking of ways to make this more formal and have the components themselves support the changes we need
That's fine, since we're making the changes we need across the entire cluster's resources, so everything will be consistent and subsequent revisions should be OK
Not for this particular thing, but in general our work is tracked under this Jira label |
This change shouldn't make a difference in terms of this operator's functionality, but it will make our life's easier and our solution simpler, so if we could merge it, it would be nice for us. But it's not a must. And in general I think for anything that doesn't require human interaction YAMLs should be avoided in favor of more deterministic formats, so I guess you could consider this a positive change regardless of why we need it in particular, but that's just my subjective opinion of course |
I wonder about the "it eventually causes the kube-scheduler-operator to issue a new revision of the scheduler." part. Is there a proof the different formatting is the cause to spin a new revision? Can you provide the operator logs to see what is causing the new revision? You should see something similar to
in the logs. Normally, content of |
I haven't seen it directly, @mresvanis did
Yes
True, but I guess some controller (this?) notices the different format and re-applies with the styling from the bindata directory, and that creates a diff |
That's the other part. In the KSO case ( cluster-kube-scheduler-operator/pkg/operator/targetconfigcontroller/targetconfigcontroller.go Lines 413 to 417 in 5892968
assets/kube-scheduler/kubeconfig-cm.yaml as it is and perform string replacement. Given $LB_INT_URL is the only string that gets replaced and KSO is the only component allowed to create/update openshift-kube-scheduler CM I strongly recommend to follow the same steps. We might extend the code base to create a new package that you can import in your solution and directly invoke the same code that renders the configmap. Thus, to avoid any issues with formatting and the config representation. I strongly discourage anyone from modifying the CM through a different solution. This will make debugging more complicated as we don't assume other component modifying KSO owned files behind our back.
|
But from my POV all I have is an already "rendered" template, the kubeconfig I operate on doesn't have an easily recognizable $LB_INT_URL string that I can simply replace, so me properly parsing it as a YAML to edit the server URL makes sense. I could instead do some form of regex search and replace to find the existing URL line, but of course I would like to avoid that.
The tool we use for editing all of the cluster resources is not written in Go so a Go package would not be very helpful in our case.
I agree, this is not great, but we have no other option for now. Also note that I'm not really modifying anything that wouldn't otherwise get reconciled to the exact same state. I just want to avoid that reconciliation because it's expensive so I have to "modify in advance", while the cluster is offline. Ideally as a long term solution we should find a way to stop scattering the cluster-name and cluster-base-domain and all the URLs that are derived from them all over the place in OCP, so they can be easily changed in few places (ideally one) without causing a flood of reconciliations |
Avoid parsing YAML for now until openshift/cluster-kube-scheduler-operator#523 is fixed, use dumb regex instead
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
(This is currently a WIP PR suggestion)
Background
The
scheduler-kubeconfig
configmap managed by this operator contains aYAML formatted
kubeconfig
string. As part of a particular novel formof OCP deployment (image based upgrade/installation), we have to
programmatically modify the
server:
entry in that kubeconfig. This iseasily done by parsing the YAML, editing the server entry and
serializing the YAML back into the configmap.
Issue
Every YAML serializer has its own opinions of what YAML should look
like. When we perform the procedure described above, we get a kubeconfig
that's slightly different than what this operator outputs (different
indentation, different quoting decisions). As a result, it eventually
causes the kube-scheduler-operator to issue a new revision of the
scheduler. Rolling out the new revision takes a few minutes, which we
would prefer to avoid.
Solution
In order to solve this we suggest the change in this commit which would
be to format this kubeconfig as unopinionated compact JSON as opposed to
human readable YAML
Alternative solutions
We could have a textual "search-and-replace" for the
server:
entry inour code, but we would prefer to do it right by properly parsing,
editing then serializing it.